discuss: trie mask issue #613

0xaatif · 2024-09-09T15:05:42Z

Relates #583

I'm a bit confused by mpt_trie::trie_subsets::create_trie_subset.
AIUI any subset should NOT change the state root of a trie - it just hides subtries behind a hash indirection.

Note that #583 calls hiding subtries "masking", mpt_trie calls it "subsetting". The StateMpt::mask function is a thin (and I think low risk) wrapper around the mpt_trie functionality:

zk_evm/trace_decoder/src/typed_mpt.rs

Lines 341 to 351 in cfe6d42

    
           fn mask(&mut self, addresses: impl IntoIterator<Item = TrieKey>) -> anyhow::Result<()> { 
        
               let inner = mpt_trie::trie_subsets::create_trie_subset( 
        
                   self.typed.as_hashed_partial_trie(), 
        
                   addresses.into_iter().map(TrieKey::into_nibbles), 
        
               )?; 
        
               self.typed = TypedMpt { 
        
                   inner, 
        
                   _ty: PhantomData, 
        
               }; 
        
               Ok(()) 
        
           }

Here we branch on masking a particular subtrie:

zk_evm/trace_decoder/src/core.rs

Lines 466 to 473 in cfe6d42

    
           if !precompiled_addresses.contains(&addr.compat()) { 
        
               // TODO(0xaatif): this looks like an optimization, 
        
               //                but if it's omitted, the tests fail... 
        
               if std::env::var_os("SKIP_MASK_PRECOMPILED").is_some_and(|it| it == "true") { 
        
               } else { 
        
                   state_mask.insert(TrieKey::from_address(addr)); 
        
               } 
        
           }

I've added an SKIP_MASK_PRECOMPILED for the sake of this discussion.

If we DO mask those addresses:

$ git rev-parse --short HEAD
cfe6d42d
$ cargo test --quiet --package trace_decoder --test consistent-with-header -- --quiet
loading test vectors...done

running 14 tests
..............
test result: ok. 14 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 13.20s

Everything is ok.
If we DO NOT mask those addresses:

$ SKIP_MASK_PRECOMPILED=true cargo test --quiet --package trace_decoder --test consistent-with-header -- --quiet
loading test vectors...done
...
test result: FAILED. 4 passed; 10 failed; 0 ignored; 0 measured; 0 filtered out; finished in 12.46s

Eek! Let's see more detail about one of the failures

$ SKIP_MASK_PRECOMPILED=true cargo test --quiet --package trace_decoder --test consistent-with-header -- --exact b4_dev@1
loading test vectors...done
...
Assertion failed at trace_decoder/tests/consistent-with-header.rs:34:17:
  check!( pairs().position(|(before, after)| {
                        before.trie_roots_after.state_root != after.tries.state_trie.hash()
                    }) == None )
with expansion:
  Some(1) == None

Assertion failed at trace_decoder/tests/consistent-with-header.rs:50:17:
  check!( gen_inputs
                        .last()
                        .map(|it| it.trie_roots_after.state_root.compat()) == Some(header.state_root) )
with diff:
  Some(
<     0xb2415bb7cb3c0dea9520a546500dc54cbe444afadd85e68bd688e3a96bf69196,
>     0x038e7e3c67c9166523c4b2bae746f5d7cf72480af4294bb900f252faf05d04e9,
  )
...

I'll try and work on a smaller test case :)

This reverts commit 408273a.

This reverts commit 0407031.

…der-backend-rewrite4

This reverts commit 13304bb.

This reverts commit ac673da.

This reverts commit 592aa3a.

This reverts commit 085e97f.

…der-backend-rewrite4

Nashtare · 2024-09-10T20:14:59Z

I'm not sure I understand the replication to begin with..

zk_evm/trace_decoder/src/core.rs

Lines 466 to 473 in cfe6d42

    
           if !precompiled_addresses.contains(&addr.compat()) { 
        
               // TODO(0xaatif): this looks like an optimization, 
        
               //                but if it's omitted, the tests fail... 
        
               if std::env::var_os("SKIP_MASK_PRECOMPILED").is_some_and(|it| it == "true") { 
        
               } else { 
        
                   state_mask.insert(TrieKey::from_address(addr)); 
        
               } 
        
           }

We enter the if condition if we do NOT have a precompile address at hand, so your SKIP_MASK_PRECOMPILED flag is irrelevant here. What #488 aimed at doing was, in the case we do have a precompile, to check if it effectively is present in the initial trie witness (i.e. not hashed out), and mark it (or mask with your terminology? I'm not sure) as touched if there is a path down the trie for it.

BGluth · 2024-09-19T16:31:38Z

Yeah creating a trie subset should always produce a trie with an identical hash, so that's a bug.

Maybe subset isn't the best name here. It's true that the trie has only subset of the original trie's node and it's a different trie, but it's a bit confusing as well since the trie is the same except for hash nodes.

I would just run this through trie diff and see where they diverge.

0xaatif and others added 30 commits September 2, 2024 14:15

mark: 0xaatif/trace-decoder-backend-rewrite4

036fd1f

refactor: initial implementation

1b89a35

refactor: switcheroo

3fdd2bc

fix: batch

844971f

refactor: trace_decoder tests do not expect json array in files

0407031

chore: trace_decoder test inputs are no longer arrays

408273a

feat: trace_decoder/examples/dump.rs

c83f223

Revert "chore: trace_decoder test inputs are no longer arrays"

6771d0c

This reverts commit 408273a.

Revert "refactor: trace_decoder tests do not expect json array in files"

2a6d843

This reverts commit 0407031.

refactor: dump accept multi

322def0

add --quiet

fd2e90d

fix: remove unaccessed storage tries

81907a9

test: ab

e05f3d0

test: break out common

4511b80

test: a/b

3546b42

fix: imp::batch

d11a365

fix: storage shenanigans

69ed607

refactor(ci): separate build for tests

69249b0

refactor(ci): refactor conditions

42b5d68

Merge remote-tracking branch 'origin/develop' into 0xaatif/trace-deco…

ace2af1

…der-backend-rewrite4

chore: change CI names

085e97f

chore: cargo run --quiet in stdio.sh

ece25d3

fix: special case empty txn_info

13304bb

Revert "fix: special case empty txn_info"

1dbc67a

This reverts commit 13304bb.

fix?: special case dummy batch

ac673da

Revert "fix?: special case dummy batch"

5be5357

This reverts commit ac673da.

try: do not run cancun hook on dummy txns

592aa3a

Revert "try: do not run cancun hook on dummy txns"

211bcbf

This reverts commit 592aa3a.

fix: do not increment txn numbers for dummy txns

50108d9

Revert "chore: change CI names"

2a7ac06

This reverts commit 085e97f.

0xaatif added 17 commits September 6, 2024 04:19

chore: comment

3f3fb37

fix: broken links

fd128b0

Merge remote-tracking branch 'origin/develop' into 0xaatif/trace-deco…

33d1c95

…der-backend-rewrite4

run: rm trace_decoder/src/imp_pass.rs

92fa796

chore: dep fmt

a1efa56

doc: cancun hook

ff1dd67

chore: iface -> interface

ff0b9d7

chore: doc rf

a34c3e1

chore: doc

ecb69d2

chore: imp -> core

42111c6

chore: batch_size_hint

c8f7e77

chore: just_access

c5bbe6b

Merge remote-tracking branch 'origin/develop' into 0xaatif/trace-deco…

eadf48d

…der-backend-rewrite4

review: markups

8639067

refactor: NamedConstant

ab5c74e

mark: 0xaatif/discuss-trie-mask

66bf870

chore: shim

cfe6d42

0xaatif requested review from muursh and Nashtare as code owners September 9, 2024 15:05

github-actions bot added the crate: trace_decoder Anything related to the trace_decoder crate. label Sep 9, 2024

0xaatif marked this pull request as draft September 9, 2024 15:36

prove: mpt library is not robust either :)

befc4ba

0xaatif force-pushed the 0xaatif/discuss-trie-mask branch from ba69d45 to ba7d7dd Compare September 9, 2024 18:18

wip: check-subsets

d263e91

0xaatif force-pushed the 0xaatif/discuss-trie-mask branch from ba7d7dd to d263e91 Compare September 9, 2024 18:19

0xaatif force-pushed the 0xaatif/trace-decoder-backend-rewrite4 branch 2 times, most recently from f56b00d to af9251a Compare September 11, 2024 11:03

Base automatically changed from 0xaatif/trace-decoder-backend-rewrite4 to develop September 12, 2024 16:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

discuss: trie mask issue #613

discuss: trie mask issue #613

0xaatif commented Sep 9, 2024 •

edited

Loading

Nashtare commented Sep 10, 2024

BGluth commented Sep 19, 2024 •

edited

Loading

	fn mask(&mut self, addresses: impl IntoIterator<Item = TrieKey>) -> anyhow::Result<()> {
	let inner = mpt_trie::trie_subsets::create_trie_subset(
	self.typed.as_hashed_partial_trie(),
	addresses.into_iter().map(TrieKey::into_nibbles),
	)?;
	self.typed = TypedMpt {
	inner,
	_ty: PhantomData,
	};
	Ok(())
	}

	if !precompiled_addresses.contains(&addr.compat()) {
	// TODO(0xaatif): this looks like an optimization,
	// but if it's omitted, the tests fail...
	if std::env::var_os("SKIP_MASK_PRECOMPILED").is_some_and(\|it\| it == "true") {
	} else {
	state_mask.insert(TrieKey::from_address(addr));
	}
	}

discuss: trie mask issue #613

Are you sure you want to change the base?

discuss: trie mask issue #613

Conversation

0xaatif commented Sep 9, 2024 • edited Loading

Nashtare commented Sep 10, 2024

BGluth commented Sep 19, 2024 • edited Loading

0xaatif commented Sep 9, 2024 •

edited

Loading

BGluth commented Sep 19, 2024 •

edited

Loading